Repository navigation
Conversation
…nSync Query conversion was queued off the listener thread while snapshotsInSync posted straight to the main queue, so Dart saw the sync event first.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a dedicated serial dispatch queue per Firestore instance (snapshotDeliveryQueue) on iOS to ensure snapshots and snapshotsInSync events are delivered in the correct order. It also adds an integration test to verify this behavior. The review feedback highlights a potential memory leak where the newly introduced snapshotDeliveryQueues are never cleaned up when Firestore instances are terminated. Additionally, the reviewer suggests using addTearDown in the integration test to guarantee the cleanup of temporary Firebase apps and stream subscriptions, which would also allow for the removal of redundant manual cleanup code.
| private var transactionHandlers: [String: TransactionStreamHandler] = [:] | ||
| private let transactionLock = NSLock() | ||
| private let listenersLock = NSLock() | ||
| private var snapshotDeliveryQueues: [String: DispatchQueue] = [:] |
There was a problem hiding this comment.
The snapshotDeliveryQueues dictionary holds strong references to DispatchQueue instances created for each Firebase app and database URL combination. Since these queues are never removed when a Firestore instance is terminated or when the plugin is cleaned up, this will cause a memory leak of DispatchQueues and associated resources, especially in scenarios where apps are dynamically initialized and deleted (such as in integration tests).
Consider removing the queue from snapshotDeliveryQueues when the Firestore instance is terminated in terminate(app:completion:), and clearing the dictionary in cleanupEventListeners().
| final writerApp = await Firebase.initializeApp( | ||
| name: 'insync-writer-${DateTime.now().microsecondsSinceEpoch}', | ||
| options: Firebase.app().options, | ||
| ); |
There was a problem hiding this comment.
To prevent resource leaks if the test fails before reaching the manual cleanup at the end of the test, use addTearDown to guarantee that the temporary Firebase app is deleted.
| final writerApp = await Firebase.initializeApp( | |
| name: 'insync-writer-${DateTime.now().microsecondsSinceEpoch}', | |
| options: Firebase.app().options, | |
| ); | |
| final writerApp = await Firebase.initializeApp( | |
| name: 'insync-writer-${DateTime.now().microsecondsSinceEpoch}', | |
| options: Firebase.app().options, | |
| ); | |
| addTearDown(writerApp.delete); |
| final querySub = collection.snapshots().listen((snapshot) { | ||
| String? value; | ||
| for (final document in snapshot.docs) { | ||
| if (document.id == docId) { | ||
| value = '${document.data()['n']}'; | ||
| } | ||
| } | ||
| events.add('query:$value:${snapshot.metadata.hasPendingWrites}'); | ||
| }); | ||
| final syncSub = firestore.snapshotsInSync().listen((_) { | ||
| events.add('sync'); | ||
| }); |
There was a problem hiding this comment.
Similarly, use addTearDown to ensure that the stream subscriptions are canceled even if the test fails or throws an exception.
final querySub = collection.snapshots().listen((snapshot) {
String? value;
for (final document in snapshot.docs) {
if (document.id == docId) {
value = '${document.data()["n"]}';
}
}
events.add('query:$value:${snapshot.metadata.hasPendingWrites}');
});
addTearDown(querySub.cancel);
final syncSub = firestore.snapshotsInSync().listen((_) {
events.add('sync');
});
addTearDown(syncSub.cancel);| await querySub.cancel(); | ||
| await syncSub.cancel(); | ||
| await writerApp.delete(); |
There was a problem hiding this comment.
With addTearDown registered immediately after resource creation, these manual cleanup calls at the end of the test are no longer necessary and can be safely removed.
…sInSync Query listeners were scheduled on the shared cached thread pool while snapshotsInSync ran on the main thread, so Dart could see the sync event first.
…idle A single-thread executor with no timeout would keep a thread for every Firestore instance that had listened.
…worker A core thread that times out can run a later task before an earlier one still sitting in the queue.
SelaseKay
changed the title
…ache Deleting a secondary app left its Firestore cached, so the next lookup threw FirebaseApp was deleted and the rest of the Android suite failed.
The GitHub Actions scan was blocked by the pull_request_target brownout and that run cannot be retried.
Description
Since cloud_firestore 6.7.0, a collection or query listener and
snapshotsInSync()can arrive in Dart out of order on Apple and Android. The native SDK raises the query snapshot and then the sync event. On Apple the plugin converts the query snapshot on a background queue and only then posts it to the main queue, whilesnapshotsInSync()posts to the main queue immediately. On Android the query listener is scheduled on a shared cached thread pool and posts to the main thread after conversion, whilesnapshotsInSync()is delivered on the main thread as soon as the SDK has scheduled that work. Dart can therefore see the sync event first. A document listener still emits on the main thread inside the callback the SDK queued first, so that pairing keeps the previous order.Apple gives each Firestore instance one serial
DispatchQueue. Android gives each instance one single-thread executor and passes it to both the query listener andaddSnapshotsInSyncListener. The query block converts the snapshot, then posts the Dart event. The sync block posts its Dart event after that work, so the events stay in SDK order. Conversion stays off the main thread. That serialization also means a large snapshot delays other query listeners on the same instance. There is no public API or Pigeon change.The same Android executor also stops an older query snapshot overtaking a newer one. On the shared pool, two snapshots of one query could convert at the same time and the slower one could be posted last. They now wait on one first-in, first-out thread. A per-listener queue would keep that query ordered, but the sync event would still be posted before conversion finished, so this pull request keeps the shared executor.
iOS and macOS share the Swift sources. The regression test runs on Apple and Android. On macOS it failed on the unfixed handlers with
write 1: [sync, query:1:false]and passed with this change, together with the existing documentsnapshotsInSync()test. On an Android emulator it failed on the unfixed handlers withwrite 3: [sync, query:3:false]and passed with this change, again with the document test.dart analyze . --fatal-infosreported no issues incloud_firestoreand its example. The formatter reported no issues.Not run: an iOS device or simulator, the rest of
melos run analyze, the full cloud_firestore end-to-end suite, and cloud_firestore 6.6.0. This change does not touch Windows or web. Those handlers post the query event before they return to the SDK, so the sync event cannot overtake it.Related Issues
snapshotsInSync()behind the query snapshot.Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]).This will ensure a smooth and quick review process. Updating the
pubspec.yamland changelogs is not required.///).melos run analyze) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?
Test plan
flutter test integration_test/e2e_test.dart --name "snapshotsInSync" -d macosfrompackages/cloud_firestore/cloud_firestore/example. The new test expects each of 24 remote query events before the matchingsnapshotsInSync()event. The existing documentsnapshotsInSync()test is in the same run.write 1: [sync, query:1:false].write 3: [sync, query:3:false], and bothsnapshotsInSynctests passed after the Android change.cloud_firestore7.1.0 delivered older snapshots after newer ones: 55, 59, 64, 41, and 45 backward deliveries of 301. The same app built against this branch delivered 0 backward in all five rounds. None of the published rounds ended on a stale value.melos run analyzefor packages other thancloud_firestoreand its example.